Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send kafka message in batch #1708

Closed
wants to merge 2 commits into from
Closed

Send kafka message in batch #1708

wants to merge 2 commits into from

Conversation

RoySunnySean007
Copy link

@CLAassistant
Copy link

CLAassistant commented Nov 4, 2020 •
edited

CLA assistant check
All committers have signed the CLA.

@mstoykov
Copy link
Collaborator

mstoykov commented Nov 4, 2020

Hi @RoySunnySean007 , can you:

  1. delete instead of commenting the code
  2. fix the linter errors
  3. give some numbers on how much of an improvement this makes. I am not sayign it doesn't have an improvement, but it will be nice for it to be measured so we can put them in the release notes.

Also unfortunately we release v0.29.0 on Monday ... by plan .. hopefully. And this just comes way too late to be included, probably. So I am just going to mark this for v0.30.0 and hopefully we will merge it early on in the release cycle :)

@mstoykov mstoykov added this to the v0.30.0 milestone Nov 4, 2020
@mostafa mostafa self-requested a review November 4, 2020 10:16
@mostafa
Copy link
Member

mostafa commented Nov 4, 2020

Hey @RoySunnySean007,

Nice job! 👍 How will this bulk sending of messages to Kafka deal with logging of any sorts, since you've effectively removed the logging code?

@codecov-io
Copy link

codecov-io commented Nov 4, 2020 •
edited

Codecov Report

Merging #1708 into master will increase coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1708      +/-   ##
==========================================
+ Coverage   71.37%   71.42%   +0.05%     
==========================================
  Files         176      176              
  Lines       13679    13675       -4     
==========================================
+ Hits         9764     9768       +4     
+ Misses       3304     3296       -8     
  Partials      611      611
Flag Coverage Δ
ubuntu 71.40% <66.66%> (+0.07%) ⬆️
windows 69.97% <66.66%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
stats/kafka/collector.go 58.62% <66.66%> (+7.00%) ⬆️
stats/cloud/collector.go 80.44% <0.00%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aeb4c86...8d347b5. Read the comment docs.

@RoySunnySean007
Copy link
Author

RoySunnySean007 commented Nov 4, 2020 •
edited

Dear @mstoykov & @mostafa ,

  1. @mstoykov I have updated the patch file, pls kindly help to have a check.
  2. This change was made one year ago on v0.26 by my colleague. To the best of our recollection, it will take 30 mins to finish sending all the k6 raw data result to kafka, load test throughput should be less than 1k/s. After we use batch mode, there is no delay.

Sorry we raised this PR a bit late...we lost some data...

Thanks
Roy

@RoySunnySean007
Copy link
Author

Dear @mostafa,

Could u pls kindly help to review it? Any comment is appreciated, thx!

mostafa
mostafa reviewed Nov 5, 2020
View reviewed changes
stats/kafka/collector.go
for _, sample := range formattedSamples {
msg := &sarama.ProducerMessage{Topic: c.Config.Topic.String, Value: sarama.StringEncoder(sample)}
partition, offset, err := c.Producer.SendMessage(msg)
if err != nil {
c.logger.WithError(err).Error("Kafka: failed to send message.")
Copy link
Member

@mostafa mostafa Nov 5, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to have logging while using c.Producer.SendMessages(msgList) on Line 165?

mostafa
mostafa reviewed Nov 5, 2020
View reviewed changes
stats/kafka/collector.go
}
// Send message in batch
c.Producer.SendMessages(msgList)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to have logging while using c.Producer.SendMessages(msgList) on Line 165?

That is, what happens if this line somehow fails?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Let me have a check. Thx @mostafa !

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RoySunnySean007 any movement on this change?

@na-- na-- modified the milestones: v0.30.0, v0.31.0 Jan 13, 2021
@na-- na-- modified the milestones: v0.31.0, v0.32.0 Feb 24, 2021
@mstoykov
Copy link
Collaborator

mstoykov commented Apr 6, 2021

Thanks for all the work done here, but given that we are:

  1. depricateing kafka output
  2. have moved it out as an extension

I will close this PR and ask that if you want to add this functionality to do so in the extension repo.

@mstoykov mstoykov closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mostafa mostafa mostafa left review comments

@mstoykov mstoykov mstoykov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.32.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@RoySunnySean007 @CLAassistant @mstoykov @mostafa @codecov-io @na--